http: add optimizeEmptyRequests option for IncomingMessage._dump #59778

RafaelGSS · 2025-09-05T20:37:49Z

Backportable version of #59747

cc: @ronag

nodejs-github-bot · 2025-09-05T20:37:55Z

Review requested:

@nodejs/http
@nodejs/net

codecov · 2025-09-05T21:32:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.43%. Comparing base (24ded11) to head (29753ac).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59778      +/-   ##
==========================================
- Coverage   88.45%   88.43%   -0.03%     
==========================================
  Files         703      703              
  Lines      207544   207577      +33     
  Branches    40013    40016       +3     
==========================================
- Hits       183587   183574      -13     
- Misses      15965    15996      +31     
- Partials     7992     8007      +15

Files with missing lines	Coverage Δ
lib/_http_incoming.js	`99.35% <100.00%> (+0.01%)`	⬆️
lib/_http_server.js	`97.35% <100.00%> (-0.03%)`	⬇️

... and 46 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nodejs-github-bot · 2025-09-06T11:17:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69091/

marco-ippolito · 2025-09-06T19:37:05Z

lib/_http_server.js

+  const fastDump = options.fastDump;
+  if (fastDump !== undefined)
+    validateBoolean(fastDump, 'options.fastDump');
+  this[kfastDump] = fastDump || false;


The OR seems redudant since we are passing through validateBoolean

How so? if fastDump is undefined, this[kfastDump] will be undefined instead of false.

You could shortcut that just a bit by assigning a default:

const { fastDump = false } = options.fastDump; validateBoolean(fastDump, 'options.fastDump'); this[kfastDump] = fastDump;

doc/api/http.md

lib/_http_server.js

pimterry · 2025-09-08T10:10:25Z

Thinking more about the heuristics mentioned at the end #59747, it looks like we can go further and do even better here, very easily.

I think we can do this for all methods, quickly & safely, because it's already impossible to receive a request with a body without it being indicated it in the headers (content-length or transfer-encoding), even in the current implementation, even for POST and others etc.

On Node v24 right now, if you send:

POST / HTTP/1.1
Host: test

message body

Then Node currently fires request and end (but no data), and then returns a 400 response to the unparseable message body (which it expects to be $METHOD $PATH HTTP/1.1 starting a new request). That's because the RFC possibilities for sending request bodies always require either content-length or transfer-encoding to be set to warn you there's a body coming, and our parsing already requires this as well.

That means we could safely apply this optimization to all cases, whenever there's no content-length or transfer-encoding header set (we could look for chunked or check for != 0, but since it's primarily a performance optimization I think a quicker simpler check is fine to start with). We already have the headers parsed here so that's very quick & easy. That lets us apply the same performance boost to POSTs and all other requests with an empty body (which is reasonably common I think - there's plenty of REST APIs where the URL defines the operation, but the request is a POST because it's not safe).

This would still be breaking behaviour, because it doesn't emit end or close events, but as an opt-in option (implicitlyCloseEmptyRequests or similar) then that's fine. Or we could check for event listeners and just manually emit the events if any are registered, in which case this wouldn't be breaking at all AFAICT, so we could apply the optimization to everybody for free, but I don't know if the overhead of that negates too much of the perf boost.

RafaelGSS · 2025-09-08T16:14:16Z

Good one @pimterry. I'll implement those later today and the tests asked by @Flarna.

RafaelGSS · 2025-09-09T12:57:31Z

Just pushed a new version (amended) that replaces the fastDump with optimizeEmptyRequest option and as suggested by @pimterry, we'll dump the IncomingMessage whenever this option is true and when there's no content-length/transfer-encoding or the method is HEAD/GET.

PTAL.

Flarna · 2025-09-09T13:36:46Z

lib/_http_server.js

+    // stream.Readable life cycle rules. The downside is that this will
+    // break some servers that read bodies for methods that don't have body headers.
+    req._dumped = true;
+    req._readableState.ended = true;


Could we move the modifications of stream internals into stream files?
I wonder also that endEmitted/ closeEmitted are set here. Are they really emitted?

I wonder also that endEmitted/ closeEmitted are set here. Are they really emitted?

They are as if they were already emitted.

lib/_http_server.js

doc/api/http.md

RafaelGSS · 2025-09-09T18:50:10Z

Applied the suggestions and added a test to guarantee .end event isn't emitted if the flag is true. I will do a second round of performance comparison on Fastify (Express doesn't seem to use req object as I thought it would use it - so the result wasn't so dramatic).

PTAL @Flarna @pimterry

test/parallel/test-http-server-optimize-empty-requests.js

lib/_http_incoming.js

Flarna · 2025-09-09T20:52:49Z

@lpinca commented on the previous PR, maybe worth to ask them for a look a this one.

mcollina

Unfortunately this new option is not easy to turn on by default because it's blocking a valid case, the HTTP/1.0 case where a request is made and the socket half closed to signal its termination. I would rather have it enabled based on the HTTP method instead.

pimterry · 2025-09-12T05:57:16Z

it's blocking a valid case, the HTTP/1.0 case where a request is made and the socket half closed to signal its termination.

@mcollina I don't think that case exists (or has ever existed). You might be thinking of the response case, where you can use connection closure to delimit bodies without using any framing headers? That's not possible for requests though, and response behaviour doesn't affect this PR. For requests, the HTTP/1.0 RFC says:

HTTP/1.0 requests containing an entity body must include a valid Content-Length header field.

and later:

Closing the connection cannot be used to indicate the end of a request body, since it leaves no possibility for the server to send back a response. Therefore, HTTP/1.0 requests containing an entity body must include a valid Content-Length header field.

I'm not even sure it would be technically possible - if you could just send content + TCP half-close to send a request body, there would be know way to ever know if a request was finished without the half-close. On any still-open connection, there could always be more request body to come.

I'd be very interested to see a counter-example, but I've just tested with a bunch of raw requets via socat and I can't see any way to make Node currently accept a request body like this, so I think we can ignore it. Same applies for HTTP/0.9.

Unfortunately this new option is not easy to turn on by default

Totally agree we can't turn this on by default anyway though, since it's a significant breaking change that will affect a lot of web frameworks & middleware etc and cause very confusing issues.

It would be great to find a way to enable this everywhere, but doing that without breakage would require emulating the actual stream behaviour (at the least, firing readable & end & close events, and probably simulating the initial paused-readable behaviour as well) and I suspect that would negate a lot of the benefit. We could explore this separately in future later if it seems worthwhile, it would definitely be good to do if we can find a way.

In the meantime, releasing the current approach allows the ecosystem to start adding the readableEnded logic to support this optimization safely now, with no downside, and if that happens widely enough we might be able to enable as-is in the distant future.

pimterry · 2025-09-12T06:13:56Z

All LGTM now (thanks @RafaelGSS!).

Just one last thing worth noting here: when this optimization is applied, it means you also can't listen to req.on('close'), which is currently the standard way to spot aborted requests where the connection is closed before the response is completed, so you can potentially skip processing requests the client is no longer interested in.

I think that's fine (you can listen for close on the response instead, or on the socket (HTTP/1) or stream (HTTP/2) directly) but it's worth being aware of.

mcollina

lgtm

bjohansebas · 2025-09-14T15:39:23Z

on-finished would technically break with these changes, since it's not detecting when IncomingMessage has completed (https://github.com/jshttp/on-finished/blob/d2974f5a18f468ea56f58acb2f6d402f4b5142f0/index.js#L92). Do you know of any patch to fix it from on-finished and also be able to complete expressjs/express#6742?

jasnell · 2025-09-15T12:12:48Z

test/parallel/test-http-server-optimize-empty-requests.js

+  }
+  res.writeHead(200);
+  res.end('ok');
+});


nit: it's not clear in this test what "be optimized" means. Expanding on some code comments could help provide context to anyone coming into this code later.

nodejs-github-bot · 2025-09-15T19:28:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69241/

RafaelGSS · 2025-09-16T20:53:32Z

PTAL @pimterry @mcollina @jasnell @ronag I had to push a small fix

anonrig · 2025-09-16T20:58:16Z

doc/api/http.md

+    or `Transfer-Encoding` headers (indicating no body) will be initialized with an
+    already-ended body stream, so they will never emit any stream events
+    (like `'data'` or `'end'`). You can use `req.readableEnded` to detect this case.
+    This option is still under experimental phase.


If this is indeed true, we should emit an experimental warning whenever this flag is used. If this is no longer correct, we should remove this comment.

I don't think emitting a warning for this specific option would be good. I imagine this being enabled on frameworks like fastify and express, and having such a warning might point users away. I am confident that the documentation note is enough for now.

What do you think? @mcollina @ronag

Than I recommend emitting a warning if it's not inside node_modules?

I have added the experimental documentation here only for the next two v24 releases at least, just in case we are missing some specific edge case that would completely change how this flag works - you know, touching on http usually breaks people in many ways. Ideally, this line should be removed in a couple of releases.

I'm not sure if it's worth it to consider such a warning since we'll remove it very soon.

@anonrig I have removed the experimental comment.

lib/_http_server.js

Signed-off-by: RafaelGSS <[email protected]> Co-Authored-By: RafaelGSS <[email protected]>

This removes the potential TOCTOU condition that will make this test flaky Signed-off-by: RafaelGSS <[email protected]>

nodejs-github-bot · 2025-09-23T14:28:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69338/

nodejs-github-bot · 2025-09-23T19:43:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69342/

RafaelGSS · 2025-09-24T20:26:55Z

Ping @anonrig

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Sep 5, 2025

RafaelGSS force-pushed the opt-in-faster-dump branch from 7961667 to 6e5fdcb Compare September 5, 2025 20:38

ronag approved these changes Sep 6, 2025

View reviewed changes

gurgunday approved these changes Sep 6, 2025

View reviewed changes

ronag requested a review from mcollina September 6, 2025 12:56

marco-ippolito reviewed Sep 6, 2025

View reviewed changes

Flarna reviewed Sep 7, 2025

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

jasnell reviewed Sep 7, 2025

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

jasnell reviewed Sep 7, 2025

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

RafaelGSS force-pushed the opt-in-faster-dump branch from 6e5fdcb to 2d617ab Compare September 9, 2025 12:56

RafaelGSS requested review from ronag, jasnell and Flarna September 9, 2025 12:57

Flarna reviewed Sep 9, 2025

View reviewed changes

ronag reviewed Sep 9, 2025

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

pimterry requested changes Sep 9, 2025

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

doc/api/http.md Show resolved Hide resolved

RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 9, 2025

RafaelGSS changed the title ~~http: optimize IncomingMessage._dump~~ http: add optimizeEmptyRequests option for IncomingMessage._dump Sep 9, 2025

ronag approved these changes Sep 9, 2025

View reviewed changes

Flarna reviewed Sep 9, 2025

View reviewed changes

test/parallel/test-http-server-optimize-empty-requests.js Outdated Show resolved Hide resolved

Flarna reviewed Sep 9, 2025

View reviewed changes

lib/_http_incoming.js Show resolved Hide resolved

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2025

This comment was marked as outdated.

Sign in to view

mcollina reviewed Sep 11, 2025

View reviewed changes

pimterry approved these changes Sep 12, 2025

View reviewed changes

mcollina approved these changes Sep 12, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

jasnell reviewed Sep 15, 2025

View reviewed changes

jasnell approved these changes Sep 15, 2025

View reviewed changes

RafaelGSS force-pushed the opt-in-faster-dump branch from a2b6157 to e33471d Compare September 15, 2025 18:54

This comment was marked as outdated.

Sign in to view

RafaelGSS force-pushed the opt-in-faster-dump branch from e33471d to e6cd7d2 Compare September 15, 2025 19:26

RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 16, 2025

anonrig requested changes Sep 16, 2025

View reviewed changes

anonrig reviewed Sep 16, 2025

View reviewed changes

lib/_http_server.js Show resolved Hide resolved

ronag and others added 5 commits September 23, 2025 11:22

http: add optimizeEmptyRequests server option

d7ee9ea

Signed-off-by: RafaelGSS <[email protected]> Co-Authored-By: RafaelGSS <[email protected]>

fixup! http: add optimizeEmptyRequests server option

78a0e8f

fixup! http: add optimizeEmptyRequests server option

58dcdd0

fixup! fixup! http: add optimizeEmptyRequests server option

3f6851c

This removes the potential TOCTOU condition that will make this test flaky Signed-off-by: RafaelGSS <[email protected]>

fixup! fixup! fixup! http: add optimizeEmptyRequests server option

29753ac

RafaelGSS force-pushed the opt-in-faster-dump branch from e6cd7d2 to 29753ac Compare September 23, 2025 14:26

ronag approved these changes Sep 23, 2025

View reviewed changes

Uh oh!

http: add optimizeEmptyRequests option for IncomingMessage._dump #59778

Are you sure you want to change the base?

http: add optimizeEmptyRequests option for IncomingMessage._dump #59778

Conversation

RafaelGSS commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 5, 2025

Uh oh!

codecov bot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Sep 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pimterry commented Sep 8, 2025

Uh oh!

RafaelGSS commented Sep 8, 2025

Uh oh!

RafaelGSS commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RafaelGSS commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Flarna commented Sep 9, 2025

Uh oh!

This comment was marked as outdated.

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

pimterry commented Sep 12, 2025

Uh oh!

pimterry commented Sep 12, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

bjohansebas commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Sep 15, 2025

Uh oh!

RafaelGSS commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RafaelGSS commented Sep 5, 2025 •

edited

Loading

codecov bot commented Sep 5, 2025 •

edited

Loading

RafaelGSS commented Sep 9, 2025 •

edited

Loading

bjohansebas commented Sep 14, 2025 •

edited

Loading

RafaelGSS commented Sep 16, 2025 •

edited

Loading